From 047b60b96d8218eec5122dbca38e367773267d4b Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 23 Sep 2015 02:35:38 +0100 Subject: [PATCH] resourceloader: Store relative instead of absolute paths in module_deps Make paths stored in the module_deps table relative to $IP. This ensures that when the MediaWiki install path changes and/or if the location of the extension or skins directory changes, that ResourceLoader's internal model is still accurate. Previously when these paths change, ResourceLoader would exhibit various bugs. 1. Unable to detect changes in the module (if the directory no longer exists). 2. Point #1 is usually preceeded by one last cache invalidation as the content hash of the file path changes (from a valid hash to an empty string). 3. Unnecessary cache invalidation (if both old and new directories exist). This happens when a file is both an entry point (in the 'scripts' or 'styles' array) and also a file dependency. At first they are de-duplicated by array_unique. But after the disk path changes, the next check will result in the old path being fetched from module_deps, and the new path from the live configuration. This causes two changes that result in needless cache invalidation: - The hash list contains one more item (T111481). - The hash list contains both the old and new version of a file. (or even alternate versions, e.g. when a change is backported to the old wmf branch; it also affects wikis on the new branch due to the stale file path still in the database). It seems unusual to move a MediaWiki install, and usually we recommend third parties to run update.php if site administrators do move their wiki. However Wikimedia's deployment system implicitly moves the MediaWiki install continously from e.g. "/srv/mediawiki/php-1.26wmf5" to "/srv/mediawiki/php-1.26wmf6". This caused virtually all ResourceLoader caching layers to get invalidated every week when another wmf-branch is deployed, thus breaking these file paths, which changes the version hash, which then invalidates the cache. Bug: T111481 Change-Id: I173a9820b3067c4a6598a4c8d77e239797d2659c --- composer.json | 1 + .../ResourceLoaderFileModule.php | 1 - .../resourceloader/ResourceLoaderModule.php | 42 ++++++++++++++++++- .../ResourceLoaderModuleTest.php | 37 ++++++++++++++++ 4 files changed, 78 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index 718a804a5c..2fadfea284 100644 --- a/composer.json +++ b/composer.json @@ -30,6 +30,7 @@ "wikimedia/cldr-plural-rule-parser": "1.0.0", "wikimedia/composer-merge-plugin": "1.2.1", "wikimedia/ip-set": "1.0.1", + "wikimedia/relpath": "1.0.3", "wikimedia/utfnormal": "1.0.3", "wikimedia/wrappedstring": "2.0.0", "zordius/lightncandy": "0.21" diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php index 0df28929ce..ad6903acef 100644 --- a/includes/resourceloader/ResourceLoaderFileModule.php +++ b/includes/resourceloader/ResourceLoaderFileModule.php @@ -416,7 +416,6 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { $context ); // Collect referenced files - $this->localFileRefs = array_unique( $this->localFileRefs ); $this->saveFileDependencies( $context->getSkin(), $this->localFileRefs ); return $styles; diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 1551ae3b64..17e2696201 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -397,7 +397,9 @@ abstract class ResourceLoaderModule { ); if ( !is_null( $deps ) ) { - $this->fileDeps[$skin] = (array)FormatJson::decode( $deps, true ); + $this->fileDeps[$skin] = self::expandRelativePaths( + (array)FormatJson::decode( $deps, true ) + ); } else { $this->fileDeps[$skin] = array(); } @@ -426,6 +428,10 @@ abstract class ResourceLoaderModule { * @param array $localFileRefs List of files */ protected function saveFileDependencies( $skin, $localFileRefs ) { + // Normalise array + $localFileRefs = array_values( array_unique( $localFileRefs ) ); + sort( $localFileRefs ); + try { // If the list has been modified since last time we cached it, update the cache if ( $localFileRefs !== $this->getFileDependencies( $skin ) ) { @@ -434,7 +440,8 @@ abstract class ResourceLoaderModule { array( array( 'md_module', 'md_skin' ) ), array( 'md_module' => $this->getName(), 'md_skin' => $skin, - 'md_deps' => FormatJson::encode( $localFileRefs ), + // Use relative paths to avoid ghost entries when $IP changes (T111481) + 'md_deps' => FormatJson::encode( self::getRelativePaths( $localFileRefs ) ), ) ); } @@ -443,6 +450,37 @@ abstract class ResourceLoaderModule { } } + /** + * Make file paths relative to MediaWiki directory. + * + * This is used to make file paths safe for storing in a database without the paths + * becoming stale or incorrect when MediaWiki is moved or upgraded (T111481). + * + * @since 1.26 + * @param array $filePaths + * @return array + */ + protected static function getRelativePaths( Array $filePaths ) { + global $IP; + return array_map( function ( $path ) use ( $IP ) { + return RelPath\getRelativePath( $path, $IP ); + }, $filePaths ); + } + + /** + * Expand directories relative to $IP. + * + * @since 1.26 + * @param array $filePaths + * @return array + */ + protected static function expandRelativePaths( Array $filePaths ) { + global $IP; + return array_map( function ( $path ) use ( $IP ) { + return RelPath\joinPath( $IP, $path ); + }, $filePaths ); + } + /** * Get the last modification timestamp of the messages in this module for a given language. * @param string $lang Language code diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php index e8ca2a3042..145698cfeb 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php @@ -91,4 +91,41 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase { 'Leave valid scripts as-is' ); } + + /** + * @covers ResourceLoaderModule::getRelativePaths + * @covers ResourceLoaderModule::expandRelativePaths + */ + public function testPlaceholderize() { + $getRelativePaths = new ReflectionMethod( 'ResourceLoaderModule', 'getRelativePaths' ); + $getRelativePaths->setAccessible( true ); + $expandRelativePaths = new ReflectionMethod( 'ResourceLoaderModule', 'expandRelativePaths' ); + $expandRelativePaths->setAccessible( true ); + + $this->setMwGlobals( array( + 'IP' => '/srv/example/mediawiki/core', + ) ); + $raw = array( + '/srv/example/mediawiki/core/resources/foo.js', + '/srv/example/mediawiki/core/extensions/Example/modules/bar.js', + '/srv/example/mediawiki/skins/Example/baz.css', + '/srv/example/mediawiki/skins/Example/images/quux.png', + ); + $canonical = array( + 'resources/foo.js', + 'extensions/Example/modules/bar.js', + '../skins/Example/baz.css', + '../skins/Example/images/quux.png', + ); + $this->assertEquals( + $getRelativePaths->invoke( null, $raw ), + $canonical, + 'Insert placeholders' + ); + $this->assertEquals( + $expandRelativePaths->invoke( null, $canonical ), + $raw, + 'Substitute placeholders' + ); + } } -- 2.20.1